AttachmentReader::read() returns ReadStatus::CLOSED if an AttachmentWriter has not been created yet

[lrvega]

Now that SharedDataStreams exist and have provided the foundation for asynchronous read/write semantics for Attachments, an AttachmentReader can be created before its corresponding AttachmentWriter. The user of the reader could call read() before the writer exists. This will return a ReadStatus::CLOSED.

In a non-blocking AttachmentReader, I would have expected it to return ReadStatus::OK_WOULDBLOCK. When it's blocking, I would have expected ReadStatus::OK_TIMEDOUT. In both cases, allowed the user of the API to decide what to do. The problem with it returning ReadStatus::CLOSED when a writer never existed is that you don't know if the writer is done unless you keep track in your code (for example, a boolean keeping track if it's the first time you've read).

I am not necessarily saying that this is a bug if this is the design intent, but I want to get confirmation.

[kencecka]

Hi Irvega,

The behavior you are observing is the intended behavior. The SDS is designed such that you should create a writer before trying to read from the SDS, otherwise SDS will report CLOSED.

At the Attachment layer, it looks like there is a potential race condition in the SDK. Have you observed a real problem here, or are you just reviewing the code? An SDS user needs to ensure that they create the writer before they try to read, but if you are writing a CapabilityAgent, you have no visibility or control over when the writer is created. In theory, a Directive might come in from AVS and have a delay before the attachment arrives, and if you tried to start reading in the mean time, it would appear that the attachment is closed.

We will review the code further to confirm that this is an issue, and identify a fix in AttachmentManager which ensures that the writer exists before the AttachmentReader is returned.

Ken

[lrvega]

Hi Ken,

Thanks for the response. That potential race condition that you mention is actually possible and we have hit it. Our directive processing thread (MessageRouterObserverInterface implementation) receives the directive and calls AttachmentManager::generateAttachmentId() and AttachmentManager::createReader() before the MimeParser in HTTP2Stream has had a chance to create the writer. That part of our application is not creating a SharedDataStream for the reading of attachments as that is done by AttachmentManager::createReader(). This will return a reader regardless of whether or not the writer exists. The MessageRouterObserverInterface implementor is not called again when the attachment is actually created by the writer. We are currently only using those building blocks (MessageRouterObserverInterface, AttachmentManager, AttachmentReader), to process an inbound message asynchronously. I didn't see a way to prevent the reader from trying to create the attachment before the writer. The low level API does not seem to provide those semantics (I could be missing something :) ) I guess that in order for those semantics to work, the MessageRouter would need to know that an attachment is available before making the directive available to the MessageRouterObserverInterface. We have have a working model now (if we haven't read anything from the attachment and we get a CLOSED return code we handle it like a WOULDBLOCK).

Thanks,
Luis

[kencecka]

Hi Luis,

Thanks for confirming that you're seeing this race condition occur. Your workaround sounds viable, and I agree with your interpretation of the issue. We are working on a fix which will address this internally, so that you will be able to safely read without any possibility of a CLOSED return.

Ken

[kencecka]

Hi Luis,

We reviewed a couple of options here and settled on changing the behavior of SDS to treat a read() from an SDS where no data has been written (irrespective of the presence of a writer) as an open/empty stream, such that the read will either block or return WOULDBLOCK (depending on the policy).

This change fixes the problem you are seeing. We will include this fix in a future release of the SDK, but if you would like to make the change in your local codebase now, change this line to read:

        if (header->writeEndCursor > 0 && !header->isWriterEnabled) {

Note that this change will cause a failure in the SDS unit tests, which can be corrected by changing these two lines to read:

    ASSERT_EQ(blocking->read(readBuf, WORDCOUNT, TIMEOUT), Sds::Reader::Error::TIMEDOUT);
    ASSERT_EQ(nonblocking->read(readBuf, WORDCOUNT), Sds::Reader::Error::WOULDBLOCK);

Thanks for pointing this issue out.
Ken

[lrvega]

Thanks Ken. We will make the appropriate changes on our side.

[mradulan]

Hi Luis,
The issue has been fixed in the latest release 0.5. Can you check if it works for you?

Thanks.

[lrvega]

Hi,

Once we've integrated the new drop and re-tested, I'll close.

Thanks,
Luis

[mradulan]

Coming the issue for now, please re-open if the problem still exists.